refactor: replace 2 DB-busy retry loops with tenacity (#1132) - #1282
Conversation
Part of #1132 (axis 2/7 of #1130). Registry: #782 — retry batch, DB-busy group. Follows up #1198 (Counter/base64 safe subset), which deferred all retry findings for per-case owner review. Replaces the two genuine hand-rolled retry loops among the 14 detector findings with tenacity.AsyncRetrying (already a direct dependency; same style as error_recovery_service.py / production_limits_service.py). The other 12 findings are poll/pacing/work-distribution loops — adjudication table in the PR, no code changes. 1. Database._with_busy_retry (src/database/facade.py) — SQLite-busy retry on the shared write path. Behaviour preserved exactly: - fixed delay ladder from self._busy_retry_delays_sec via wait_chain (no jitter, no exponent); - attempts = len(delays)+1 via stop_after_attempt; - non-busy OperationalError re-raised immediately; - same per-retry/final warning logs; final failure raises DatabaseBusyError chained to the last busy error. 2. TelegramCommandDispatcher._update_command_safely — unbounded exponential backoff on DatabaseBusyError. Behaviour preserved exactly: - ladder INITIAL, 2x, 4x... capped at MAX via wait_exponential; - retry_busy=False -> single attempt, busy swallowed; - non-busy exceptions swallowed (status updates never kill the loop); - sleep=asyncio.sleep keeps the existing test seam; CancelledError still propagates (tenacity re-raises BaseException). tenacity trap discovered (#1132): AsyncRetrying awaits the result of fn(), so fn must be an async callable. _with_busy_retry receives a SYNC Callable returning a coroutine (lambda: begin_immediate(...)); without the _wrap_async adapter the coroutine was never awaited, the BEGIN never ran, and "cannot COMMIT - no transaction is active" leaked out via PytestUnraisableExceptionWarning (errors under filterwarnings= ["error"]). Recorded for the #782 registry. Retry/lifecycle check (#958 trap): both loops are local SQLite retries (SQLITE_BUSY = statement did NOT execute), so a retry cannot double-apply a write. No network, no billing, no SDK retry layer added. TDD: 7 characterisation tests added first (green on old impl), then the swap landed with the tests untouched: - test_database.py: exact sleep ladder on exhaustion; result after transient busy; non-busy OperationalError raise-through (zero sleeps); DatabaseBusyError.__cause__ is the last busy error. - test_telegram_command_dispatcher.py: exponential ladder doubles+caps; retry_busy=False single-shot; generic error swallowed. Detector: findings for both functions gone; --fail-on-new green. Baseline refreshed (13 -> 12): -2 fixed; pure path renames folded in (agent/manager.py -> agent/backends/_stream.py, telegram/collector.py -> collector_mixins/*); +1 new finding recorded (web/scheduler/context.py:236, counter with two aggregates -> propose keep). Co-Authored-By: Claude <noreply@anthropic.com>
2a8a69a to
7a67287
Compare
🔍 Local review (cycle 1)Reviewed locally ( Codex verdict:
No |
#1282) Cycle-review cleanup (local, cycle 1 — 2 SKIP findings, no FIX): 1. src/database/facade.py _log_retry: compute the retry delay from our own delay ladder (delays[attempt_number-1]) instead of reading the undocumented tenacity-internal retry_state.next_action.sleep. Same logged value (verified: attempt_number is the just-failed 1-based attempt, after which we sleep delays[attempt_number-1]); no longer fragile to a tenacity upgrade removing/renaming next_action. Guarded idx bounds → 0.0 fallback. 2. tests/test_database.py: docstring on test_busy_retry_returns_action_result_ after_transient_busy now states it also guards _wrap_async (returns sentinel only if the coroutine was actually awaited — without the adapter tenacity returns an un-awaited coroutine and `result is sentinel` fails). No behaviour change; ruff clean; 641 affected tests green. Co-Authored-By: Claude <noreply@anthropic.com>
📋 Review summary — all cycles
Totals: 0 FIX, 2 SKIP (both applied as cleanup in Review binding: head |
Part of #1132 (axis 2/7 of #1130). Registry: #782 — retry batch, DB-busy group. Follows up #1198 (Counter/base64 safe subset), which explicitly deferred all retry findings for per-case owner review.
What
Replaces the two genuine hand-rolled retry loops among the 14 detector findings with
tenacity.AsyncRetrying(already a direct dependency; same style aserror_recovery_service.py/production_limits_service.py). The other 12 findings are poll/pacing/work-distribution loops — adjudication table below, no code changes.1.
Database._with_busy_retry(src/database/facade.py)SQLite-busy retry on the shared write path. Behaviour preserved exactly:
self._busy_retry_delays_secviawait_chain(*(wait_fixed(d) ...))— no jitter, no exponent;len(delays) + 1viastop_after_attempt;OperationalErrorre-raised immediately (retry_if_exception(_is_retryable_busy_error));DatabaseBusyErrorchained (from) to the last busy error.2.
TelegramCommandDispatcher._update_command_safely(src/services/telegram_command_dispatcher.py)Unbounded exponential backoff on
DatabaseBusyError. Behaviour preserved exactly:INITIAL, 2×, 4×, …capped atMAXviawait_exponential(multiplier=INITIAL, max=MAX)— bit-identical float sequence (doubling is exact in binary);retry_busy=False→ single attempt, busy swallowed with the same log line;sleep=asyncio.sleepkeeps the existing test seam — suite patches ofmod.asyncio.sleepstill observe the retry sleeps;CancelledErrorstill propagates (tenacity re-raisesBaseExceptions,except Exceptiondoesn't catch them) —test_run_loop_cancelled_reraises_when_update_busystays green.tenacity trap discovered (for the #782 registry)
AsyncRetryingawaits the result offn(), sofnmust be an async callable._with_busy_retryreceives a syncCallablereturning a coroutine (lambda: begin_immediate(...)). Without the_wrap_asyncadapter the coroutine was never awaited →BEGIN IMMEDIATEnever ran →transaction()hitcannot COMMIT - no transaction is active, leaking out asPytestUnraisableExceptionWarning(which is an error under the project'sfilterwarnings = ["error"], so it silently broke every DB write and red the whole suite). The adapter is one line (async def _runner(): return await action()); the dispatcher path is unaffected because its_updateis alreadyasync def. Worth a line in #782 so future retry→tenacity conversions don't repeat it.Retry/lifecycle check (#958 trap)
Both loops are local SQLite retries — no network, no billing.
SQLITE_BUSYmeans the statement did not execute, so a retry cannot double-apply a write. tenacity adds no hidden extra retry layer here (no SDK involved); attempt counts and delay sequences are pinned by tests.TDD / behavioural equivalence
7 characterisation tests were added first and verified green on the old implementation, then the swap landed with the tests untouched:
test_database.py: exact sleep ladder on exhaustion; result returned after transient busy (slept exactlydelays[0]); non-busyOperationalErrorraise-through with zero sleeps;DatabaseBusyError.__cause__is the last busy error.test_telegram_command_dispatcher.py: exponential ladder doubles and caps (computed from module constants);retry_busy=Falsesingle-shot without sleep; generic error swallowed without retry.Detector / baseline
--fail-on-newgreen.agent/manager.py→agent/backends/_stream.py,telegram/collector.py→telegram/collector_mixins/*— same findings, files moved by earlier refactors); +1 genuinely new finding recorded (web/scheduler/context.py:236, see table).Adjudication of the remaining 12 findings (proposal — owner decides, #782)
agent/backends/_stream.py:463_await_with_countdownservices/task_handlers/stats.pyhandle_stats_alltelegram_command_dispatcher.py_run_loopunified_dispatcher.py_run_loopcollector_mixins/collection.pycollect_all_channelscollector_mixins/stats.pycollect_all_statspool_dialogs.pywarm_all_dialogs,leave_channelspool_dialogs.pydelete_dialogsleave_channels: retry delegated torun_with_flood_wait_retry, loop iterates different dialogsweb/bootstrap.py_retry_telegram_pool_until_connectedweb/search/handlers.py_telegram_search_via_workerweb/scheduler/context.py:236_dedupe_recent_unavailability_events(NEW, counter)collections.Countercovers only the count and would split a single-pass aggregation into two structuresRegistry row (#782 journal):
database/facade.py_with_busy_retry;telegram_command_dispatcher.py_update_command_safely(DB-busy retry)Not merging — owner reviews (retry→library is in the dual-review category per the task brief).
🤖 Generated with Claude Code